-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API: add Plaform (OS and Architecture) to /containers/json #42464
base: master
Are you sure you want to change the base?
Conversation
8404686
to
1ea093e
Compare
This comment has been minimized.
This comment has been minimized.
1ea093e
to
fb4a195
Compare
fb4a195
to
76c8a60
Compare
Depends on #42063 (included) |
api/types/types.go
Outdated
@@ -54,13 +54,37 @@ type ImageMetadata struct { | |||
LastTagTime time.Time `json:",omitempty"` | |||
} | |||
|
|||
// ImagePlatform describes the platform which the image in the manifest runs on. | |||
// This type is our equivalent of ocispec.Platform, with Capitalized JSON field names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: the /distribution/{name}/json
endpoint already returns platform information, but that's a direct use of the ocispec.Platform, which means that that endpoint does not use the same capitalisation.
Should we omit this type (and use a direct copy of the OCI-spec, or change the other endpoint?
In general, I prefer that we define our own types for the API (in either case), instead of types we don't (directly) control (to prevent inadvertently changing API responses)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems strange IMO for us to be returning what's effectively an "OCI Platform" object without using the canonical capitalization for the fields, so I'd vote anywhere we return a full proper "platform object" that it be the lowercase OCI versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't have a strong opinion one way or another on using the OCI structs directly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. We can use our own types if desired but it would be nice to use the industry standard field name format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Thought I pushed 🤦
I updated to use the same casing as upstream; still using our own type (with the addition of a Stringer() interface), but otherwise same as the OCI type
76c8a60
to
6d4ebbe
Compare
f43ca2b
to
7753a5a
Compare
daemon/container.go
Outdated
// TODO do we need a "fallback" here for images that have empty OS/Arch/etc (in which case, use runtime.GOARCH?) | ||
// would be nice if img had methods for this with that implemented (img.OS(), img.Architecture(), img.Platform()) | ||
base.Platform = ocispec.Platform{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to "normalize" this information here, or keep it "as-is" and handle it on the client side? https://github.com/containerd/containerd/blob/v1.5.2/platforms/platforms.go#L265-L278
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added containerd's Normalize
code here, but perhaps this is something to be discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO doing the normalize
client-side makes sense (or doing the normalize
before we ever store/use the data in the first place 😇)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally started with that (do the normalising on the client side), then noticed that "normalising" is doing more than just changing casing and (e.g.) x86 -> amd64
. It's also filling in missing information based on the current platform it's running on (which obviously wouldn't work if the client is running on a different platform as the daemon.
From that, I was also wondering if the client should just present the information the daemon provides (in most situations i think that's the ideal), hence I moved it here.
or doing the normalize before we ever store/use the data in the first place
You mean "when we store the image" ? That felt a bit tricky; should we modify image data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg, sorry, didn't realize this was straight from the image. In that case I'd be even more in favor of the API passing the data as-is all the way through and letting the client decide whether to normalize before presentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there are cases where containerd's Normalize
will insert the current architecture's info, right? Doesn't amd64
always means linux/amd64
, even on Windows, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does normalise the OS to be the "current" OS, so potentially show darwin
or macOS
on a Mac CLI; https://github.com/containerd/containerd/blob/36cc874494a56a253cd181a1a685b44b58a2e34a/platforms/database.go#L69-L80
func normalizeOS(os string) string {
if os == "" {
return runtime.GOOS
}
os = strings.ToLower(os)
switch os {
case "macos":
os = "darwin"
}
return os
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, that's unfortunate (I guess that's a quirk of part of these APIs being designed for data storage / display and part of them being designed for querying, like the fallback vector code...) 😞
Sounds like the best thing to do in this case then is just to display as-is -- if an image is missing the os
value, we probably shouldn't inject/infer one for display, right?
@tonistiigi @tianon @cpuguy83 PTAL. I think this is ready for review, but I left some comments above about things that may need input;
I also opened a draft PR for the CLI; docker/cli#3177 (TBD whether or not |
42d695b
to
5c00664
Compare
5c00664
to
635ce3c
Compare
635ce3c
to
0c6ef8c
Compare
Very quick, 10 minute attempt at adding platform information to containers (for `docker ps`). This information is useful for (e.g.) Docker Desktop, where non-matching architectures can be run, using QEMU binfmt. Lots of things to do still (besides "design"), also considering: - filter by architecture - perhaps a way to indicate that a container/image is non-native (useful to provide that information, so that clients don't have to replicate the "variants" such as arm/v5 running on arm64 (which is not "matching", but no emulation should be needed in that case); similarly, Windows OS versions (?) - check for consistency, e.g. `docker inspect` shows a `Platform: linux`, but does not contain `Architecture`. ```bash docker run -d --name foo nginx:alpine curl --unix-socket /var/run/docker.sock http://localhost/containers/json | jq ``` ```json [ { "Id": "cf30128fc519f22d74fff234757a1592a97c05a7e1c5e411eef394d8dad571b9", "Names": [ "/foo" ], "Image": "nginx:alpine", "ImageID": "sha256:a6eb2a334a9fcf41788e0cff252f749df07427df29e50f0b141cc5ed1094006d", "Command": "/docker-entrypoint.sh nginx -g 'daemon off;'", "Created": 1622809767, "Ports": [ { "PrivatePort": 80, "Type": "tcp" } ], "Labels": { "maintainer": "NGINX Docker Maintainers <docker-maint@nginx.com>" }, "State": "running", "Status": "Up About a minute", "HostConfig": { "NetworkMode": "default" }, "Platform": { "architecture": "amd64", "os": "linux" }, "NetworkSettings": { "Networks": { "bridge": { "IPAMConfig": null, "Links": null, "Aliases": null, "NetworkID": "45a313a87b5237c5d0ddf5d5de01e44dd2940af95e74ba34ebd760ccb02acfb3", "EndpointID": "ad9557d54bb9bb63995760a911296e6ef2a6dd22c1ea65bcb58998caae6537b5", "Gateway": "172.18.0.1", "IPAddress": "172.18.0.2", "IPPrefixLen": 16, "IPv6Gateway": "", "GlobalIPv6Address": "", "GlobalIPv6PrefixLen": 0, "MacAddress": "02:42:ac:12:00:02", "DriverOpts": null } } }, "Mounts": [] } ] ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
With this change, on API v1.42: curl -s --unix-socket /var/run/docker.sock "http://localhost/v1.42/containers/json?all=1" | jq . [ { "Id": "7291db238bf9aaa02e42e09557aa5e52151a9f3b62f95049d6ce7448b5e2df78", "Names": [ "/stupefied_morse" ], "Image": "hello-world", "ImageID": "sha256:d1165f2212346b2bab48cb01c1e39ee8ad1be46b87873d9ca7a4e434980a7726", "Platform": { "Architecture": "amd64", "OS": "linux" }, "Command": "/hello", "Created": 1625405554, "Ports": [], "Labels": {}, "State": "created", "Status": "Created", "HostConfig": { "NetworkMode": "default" }, "NetworkSettings": { "Networks": { "bridge": { "IPAMConfig": null, "Links": null, "Aliases": null, "NetworkID": "", "EndpointID": "", "Gateway": "", "IPAddress": "", "IPPrefixLen": 0, "IPv6Gateway": "", "GlobalIPv6Address": "", "GlobalIPv6PrefixLen": 0, "MacAddress": "", "DriverOpts": null } } }, "Mounts": [] } ] And on API v1.41 and before: docker create hello-world curl -s --unix-socket /var/run/docker.sock "http://localhost/v1.41/containers/json?all=1" | jq . [ { "Id": "7291db238bf9aaa02e42e09557aa5e52151a9f3b62f95049d6ce7448b5e2df78", "Names": [ "/stupefied_morse" ], "Image": "hello-world", "ImageID": "sha256:d1165f2212346b2bab48cb01c1e39ee8ad1be46b87873d9ca7a4e434980a7726", "Command": "/hello", "Created": 1625405554, "Ports": [], "Labels": {}, "State": "created", "Status": "Created", "HostConfig": { "NetworkMode": "default" }, "NetworkSettings": { "Networks": { "bridge": { "IPAMConfig": null, "Links": null, "Aliases": null, "NetworkID": "", "EndpointID": "", "Gateway": "", "IPAddress": "", "IPPrefixLen": 0, "IPv6Gateway": "", "GlobalIPv6Address": "", "GlobalIPv6PrefixLen": 0, "MacAddress": "", "DriverOpts": null } } }, "Mounts": [] } ] Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This allows the platform to be printed as a string. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
0c6ef8c
to
95b43fc
Compare
Updated the test; ptal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno if this still makes sense in containerd-integration, but LGTM 😇
type: "object" | ||
x-nullable: true | ||
description: | | ||
Platform information of the image that the container was created from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this mention explicitly that this follows the OCI definition of the fields?
(Maybe we need a PR to the image-spec to add an anchor to https://github.com/opencontainers/image-spec/blob/v1.1.0/image-index.md#:~:text=generate%20an%20error.-,platform%20object,-This%20OPTIONAL%20property -- I find myself linking to it constantly with this janky browser-specific syntax 😂)
docker ps
#47764Very quick, 10 minute attempt at adding platform information to containers (for
docker ps
). This information is useful for (e.g.) Docker Desktop, where non-matching architectures can be run, using QEMU binfmt.Lots of things to do still (besides "design"), also considering:
docker inspect
shows aPlatform: linux
, but does not containArchitecture
.docker run -d --name foo nginx:alpine curl --unix-socket /var/run/docker.sock http://localhost/containers/json | jq
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)